Skip to content

feat(wrangler): createServer API (with test migrated)#13992

Closed
edmundhung wants to merge 20 commits into
mainfrom
edmundhung/create-server
Closed

feat(wrangler): createServer API (with test migrated)#13992
edmundhung wants to merge 20 commits into
mainfrom
edmundhung/create-server

Conversation

@edmundhung

Copy link
Copy Markdown
Member

Fixes #[insert GH or internal issue link(s)].

Describe your change...


  • Tests
    • Tests included/updated
    • Automated tests not possible - manual testing has been completed as follows:
    • Additional testing not necessary because:
  • Public documentation
    • Cloudflare docs PR(s):
    • Documentation not necessary because:

A picture of a cute animal (not mandatory, but encouraged)

@changeset-bot

changeset-bot Bot commented May 20, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: 571faa4

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-project-automation github-project-automation Bot moved this to Untriaged in workers-sdk May 20, 2026
@edmundhung edmundhung force-pushed the edmundhung/create-server branch from 68ec1c2 to 4aee674 Compare May 20, 2026 21:28

@ask-bonk ask-bonk Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm Bonk, and I've done a quick review of your PR.

This PR introduces a createServer API that wraps the existing DevEnv/controller infrastructure with a cleaner, migration-focused interface. It migrates existing fixture tests from unstable_startWorker and adds comprehensive e2e tests.

Issues (highest severity first)

  1. update() may resolve too early in multiworker scenarios — In a multiworker setup, waitForReloadComplete resolves on the first reloadComplete event from the primary DevEnv. But updateConfig sets each worker's config sequentially. When the primary worker's config is set and triggers a bundle, the MultiworkerRuntimeController may still have cached options for the auxiliary worker from the previous round, so #canStartMiniflare() returns true and emits reloadComplete with stale auxiliary config. The subsequent auxiliary config update triggers a second reload, but waitForReloadComplete has already resolved.

  2. close() racing with listen() can trigger assertion failure — If close() is called while listen() is still awaiting startPromise, both will resume after the promise settles. If close() runs its microtask continuation first, it clears serverSession, and then listen() hits the assert(serverSession) on line 372. A simple guard (e.g., a closed flag checked in listen()) would make this safe.

  3. Minor: BaseController.mf is now a public mutable field — The #mf private field in both LocalRuntimeController and MultiworkerRuntimeController was refactored to a public mf property on the abstract RuntimeController base class. This is fine for the server.ts use case, but it widens the API surface of RuntimeController. Consider whether a read-only accessor would be more appropriate to prevent accidental external mutation.

Comment thread packages/wrangler/src/api/server.ts Outdated
Comment on lines +466 to +478
if (serverSession) {
const nextInputs = resolveWorkerInputs(currentOptions, serverAuthHook);

if (nextInputs.length !== serverSession.devEnvs.length) {
throw new Error(
`Updating the number of workers running in the server is not supported.`
);
}

await Promise.all([
waitForReloadComplete(serverSession),
updateConfig(serverSession, nextInputs),
]);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a multiworker setup, updateConfig sets configs sequentially (primary first, then auxiliary). After the primary worker's config is set and its bundle pipeline runs, the MultiworkerRuntimeController may already have cached options for the auxiliary worker from the previous round. #canStartMiniflare() returns true, miniflare is updated with the old auxiliary config, and reloadComplete fires. waitForReloadComplete resolves here — but the auxiliary worker's config hasn't been updated yet.

When the auxiliary config is subsequently set, it triggers another bundle → reload cycle, but update() has already returned to the caller, who may now issue requests hitting stale auxiliary worker code.

This only affects multiworker update() calls. The initial listen() path doesn't have this issue because waitForPrimaryReady awaits the proxy ready promise (which is recreated on each reload).

One approach: for multiworker, wait for N reloadComplete events (or a final "all settled" signal) instead of resolving on the first one.

Comment on lines +361 to +378
async listen() {
if (!serverSession) {
if (!startPromise) {
startPromise = startServerSession().finally(() => {
startPromise = undefined;
});
}

await startPromise;
}

assert(serverSession, "Worker server has no active session.");
const ready = await serverSession.primaryDevEnv.proxy.ready.promise;

return {
url: ready.url,
inspectorUrl: ready.inspectorUrl,
};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If close() is called concurrently while listen() is awaiting startPromise, both awaiters resume after the promise resolves. If close()'s continuation runs first (clearing serverSession), the assert on line 372 will throw in listen().

This is an edge case (calling close() while listen() is in-flight is unusual), but since close() explicitly handles the pending-start case by awaiting startPromise, it seems like a scenario the code intends to support. A simple fix:

Suggested change
async listen() {
if (!serverSession) {
if (!startPromise) {
startPromise = startServerSession().finally(() => {
startPromise = undefined;
});
}
await startPromise;
}
assert(serverSession, "Worker server has no active session.");
const ready = await serverSession.primaryDevEnv.proxy.ready.promise;
return {
url: ready.url,
inspectorUrl: ready.inspectorUrl,
};
async listen() {
if (!serverSession) {
if (!startPromise) {
startPromise = startServerSession().finally(() => {
startPromise = undefined;
});
}
await startPromise;
}
if (!serverSession) {
throw new Error("Worker server was closed before it finished starting.");
}
const ready = await serverSession.primaryDevEnv.proxy.ready.promise;
return {
url: ready.url,
inspectorUrl: ready.inspectorUrl,
};
},

@ask-bonk

ask-bonk Bot commented May 20, 2026

Copy link
Copy Markdown
Contributor

Review posted on PR #13992 with two inline comments and a summary. The main findings are:

  1. update() may resolve prematurely in multiworker setupswaitForReloadComplete captures the first reloadComplete event, but sequential config updates in multiworker mode can cause the primary worker to reload with stale auxiliary config before the auxiliary update has propagated.

  2. close() racing with listen() — A concurrent close() during listen()'s startup await can cause an assertion failure. Suggested replacing the assert with a descriptive error.

  3. Minor: mf field visibility — The private #mf field was promoted to a public property on the abstract RuntimeController base class.

github run

@edmundhung edmundhung force-pushed the edmundhung/create-server branch from 4aee674 to 3d28052 Compare May 20, 2026 21:32
@pkg-pr-new

pkg-pr-new Bot commented May 20, 2026

Copy link
Copy Markdown
create-cloudflare

npm i https://pkg.pr.new/create-cloudflare@13992

@cloudflare/deploy-helpers

npm i https://pkg.pr.new/@cloudflare/deploy-helpers@13992

@cloudflare/kv-asset-handler

npm i https://pkg.pr.new/@cloudflare/kv-asset-handler@13992

miniflare

npm i https://pkg.pr.new/miniflare@13992

@cloudflare/pages-shared

npm i https://pkg.pr.new/@cloudflare/pages-shared@13992

@cloudflare/unenv-preset

npm i https://pkg.pr.new/@cloudflare/unenv-preset@13992

@cloudflare/vite-plugin

npm i https://pkg.pr.new/@cloudflare/vite-plugin@13992

@cloudflare/vitest-pool-workers

npm i https://pkg.pr.new/@cloudflare/vitest-pool-workers@13992

@cloudflare/workers-editor-shared

npm i https://pkg.pr.new/@cloudflare/workers-editor-shared@13992

@cloudflare/workers-utils

npm i https://pkg.pr.new/@cloudflare/workers-utils@13992

wrangler

npm i https://pkg.pr.new/wrangler@13992

commit: 571faa4

@edmundhung edmundhung force-pushed the edmundhung/create-server branch 6 times, most recently from 82f5aaf to 6e31e66 Compare May 26, 2026 17:41
@edmundhung edmundhung force-pushed the edmundhung/create-server branch from 7adf020 to d151055 Compare May 28, 2026 13:14
@edmundhung edmundhung force-pushed the edmundhung/create-server branch from 4f2a1a3 to 571faa4 Compare May 28, 2026 13:33
@edmundhung edmundhung changed the title feat(wrangler): createServer API feat(wrangler): createServer API (with test migrated) May 28, 2026
@edmundhung edmundhung closed this Jun 8, 2026
@github-project-automation github-project-automation Bot moved this from Untriaged to Done in workers-sdk Jun 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants